Conversation
Summary of ChangesHello @KimGyeongLock, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! 이 PR은 강의 랭킹 서비스의 로직을 개선하여, 특히 교양 과목 랭킹의 정확성과 관련성을 높이는 데 중점을 둡니다. 특정 교양 과목들을 랭킹에서 제외함으로써 사용자에게 더 유의미한 상위 랭킹 목록을 제공합니다. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
이 PR은 과목 랭킹에서 특정 과목을 제외하는 필터링 기능을 추가하는 좋은 개선입니다.
다만, 몇 가지 개선점을 제안합니다:
- PR 제목과 설명이 기여 가이드라인(Conventional Commits, PR 본문 템플릿)을 따르지 않고 있습니다. 가이드에 맞춰 제목과 설명을 수정해 주시면 좋겠습니다. (스타일 가이드 규칙 131-137, 153)
CourseRankingService에 새로운 필터링 로직이 추가되었지만, 이에 대한 단위 테스트가CourseRankingServiceTest.java에 반영되지 않았습니다. 새로운 기능이 의도대로 동작하는지, 향후 변경 시에도 문제가 없는지 확인하기 위해 테스트 케이스를 추가하는 것이 매우 중요합니다. 예를 들어 '채플'과 같은 과목이 교양 랭킹에서 올바르게 제외되는지 검증하는 테스트가 필요합니다. (스타일 가이드 규칙 152)- 코드 가독성, 유지보수성, 효율성 향상을 위한 몇 가지 제안 사항을 아래 리뷰 코멘트에 남겼습니다.
| public RankingResponse getCourseRanking() { | ||
| return new RankingResponse( | ||
| loadTop10(MAJOR_CATEGORIES), | ||
| loadTop10(LIBERAL_CATEGORIES) | ||
| loadTop10(MAJOR_CATEGORIES, false), | ||
| loadTop10(LIBERAL_CATEGORIES, true) | ||
| ); | ||
| } |
There was a problem hiding this comment.
스타일 가이드(규칙 66)에 따라 읽기 전용(read-only) 트랜잭션에는 @Transactional(readOnly = true)를 명시하는 것이 좋습니다. 이 어노테이션은 JPA에게 성능 최적화를 할 수 있다는 힌트를 주어 데이터베이스 부하를 줄일 수 있습니다. getCourseRanking 메서드는 데이터를 조회만 하므로 이 어노테이션을 추가하는 것을 권장합니다. FQCN(정규화된 클래스 이름)을 사용하면 import 문을 추가하지 않고도 적용할 수 있습니다.
| public RankingResponse getCourseRanking() { | |
| return new RankingResponse( | |
| loadTop10(MAJOR_CATEGORIES), | |
| loadTop10(LIBERAL_CATEGORIES) | |
| loadTop10(MAJOR_CATEGORIES, false), | |
| loadTop10(LIBERAL_CATEGORIES, true) | |
| ); | |
| } | |
| @org.springframework.transaction.annotation.Transactional(readOnly = true) | |
| public RankingResponse getCourseRanking() { | |
| return new RankingResponse( | |
| loadTop10(MAJOR_CATEGORIES, false), | |
| loadTop10(LIBERAL_CATEGORIES, true) | |
| ); | |
| } |
References
- 읽기 전용 쿼리는 @transactional(readOnly = true) 명시. (link)
| private List<RankingItem> loadTop10(Set<Category> categories) { | ||
| var rows = repository.findTopCoursesByCategories( | ||
| private List<RankingItem> loadTop10(Set<Category> categories, boolean applyLiberalFilter) { | ||
| int fetchSize = applyLiberalFilter ? 30 : 10; |
There was a problem hiding this comment.
30이라는 숫자가 '매직 넘버'로 사용되었습니다. 이 값이 왜 선택되었는지 코드만으로는 명확하지 않습니다. 가독성과 유지보수성을 높이기 위해 이 값을 LIBERAL_RANKING_FETCH_SIZE와 같은 의미 있는 이름의 상수로 추출하고, 값의 의도를 설명하는 주석을 추가하는 것을 권장합니다. 이렇게 하면 나중에 제외 키워드가 늘어나는 등의 변화에 대응하기 용이해집니다.
| final List<? extends CourseRankingRepository.CourseCountRow> finalRows; | ||
|
|
||
| if (applyLiberalFilter) { | ||
| finalRows = fetchedRows.stream() | ||
| .filter(r -> !containsAny(r.getName(), LIBERAL_EXCLUDE_KEYWORDS)) | ||
| .limit(10) | ||
| .toList(); | ||
| } else { | ||
| finalRows = fetchedRows.stream() | ||
| .limit(10) | ||
| .toList(); | ||
| } |
There was a problem hiding this comment.
if/else 블록에서 스트림을 생성하고 .limit(10).toList()를 호출하는 부분이 중복됩니다. 코드를 조금 더 간결하게 리팩토링할 수 있습니다. 스트림을 먼저 생성하고, 조건에 따라 필터 연산을 추가한 뒤, 마지막에 limit과 toList를 한 번만 호출하는 방식이 더 깔끔합니다.
Stream<? extends CourseRankingRepository.CourseCountRow> stream = fetchedRows.stream();
if (applyLiberalFilter) {
stream = stream.filter(r -> !containsAny(r.getName(), LIBERAL_EXCLUDE_KEYWORDS));
}
final List<? extends CourseRankingRepository.CourseCountRow> finalRows = stream.limit(10).toList();| private boolean containsAny(String text, List<String> keywords) { | ||
| if (text == null) return false; | ||
| for (String kw : keywords) { | ||
| if (text.contains(kw)) return true; | ||
| } | ||
| return false; | ||
| } |
There was a problem hiding this comment.
containsAny 메서드는 Java 스트림 API의 anyMatch를 사용하면 더 간결하고 표현력 있게 작성할 수 있습니다. 가독성 향상에 도움이 됩니다.
private boolean containsAny(String text, List<String> keywords) {
if (text == null) {
return false;
}
return keywords.stream().anyMatch(text::contains);
}
|
1 participant
.